Slevomat Coding Standard 6.x #59
Conversation
|
fixes #60 |
|
Hi, thanks for the PR (looks great!). I just wanted to try it before merging and update the commit message to match the project style, but it looks like pushes are disabled for this PR? Could you please allow them (or change the commit msg to not be in the past/passive tense?). Thanks :) |
|
@VasekPurchart Forcepushed. |
There was a problem hiding this comment.
I tested this on the 3.x branch and found out that there has been (probably undocumented - or at least I don't see it in release notes) a change/fix in SlevomatCodingStandard.Commenting.RequireOneLinePropertyDocComment.
Now it works probably for any comments? Or maybe just PHPDocs, but anyway this is no longer compatible with what is written in the standard (which uses this only for @var), so it needs to be removed.
Other option would be if there was an option on the sniff to apply this only for @var/properties, but I have checked and there probably is not.
Or am I missing something? Thanks :)
|
It always worked for any property phpdoc. So it's not related to this update. |
|
My guess was that some of the helpers might have changed the behavior , for example https://github.com/slevomat/coding-standard/commits/master/SlevomatCodingStandard/Helpers/DocCommentHelper.php ? The only changes which were changed in the project I used for testing were:
So it is possible that this is caused by a change in PHPCS (although probably less likely), but since 3.5.3 is force by 6.x it does not matter anyway.
My bad I kind of overlooked that there is property in the title :) But there is definitely a bug, which threw me off before. See 485efa0 + https://travis-ci.org/consistence/coding-standard/jobs/626363642#L305-L308 - the error is attributed to the line where a multiline (but with just one annotation) PHPDoc is starting, but which is rather a class PHPDoc. While the rest of the file is ok-ish - the missing PHPDoc property should be a different issue. The combination of these two seems to be the trigger. So I think the sniff should stay, but there is a regression, which can be fixed in a patch release, so we do not need to solve it in this release. Do you want me to create a separate issue or is the above reproducer enough? |
|
Thanks again :) |
|
You're welcome :) |
VasekPurchart
changed the title
|
Backported to 3.x and released as 3.10 . |
I'm not sure if native type hints for properties should be disabled here.